Optimize tuple management by sbckr · Pull Request #65 · carbynestack/castor

sbckr · 2024-08-26T07:23:10Z

No description provided.

- intra-vcp/tuples endpoint now returns bytes instead of JSON (breaking change) - tupleList is serialized to bytes Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

- Castor no longer uses the TupleList class and directly returns bytes retrieved from MinIO Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

codecov · 2024-08-26T07:25:02Z

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 83.24%. Comparing base (eead173) to head (31376bc).

Files	Patch %	Lines
...carbynestack/castor/common/entities/TupleList.java	0.00%	8 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master      #65      +/-   ##
============================================
- Coverage     85.07%   83.24%   -1.83%     
+ Complexity      300      286      -14     
============================================
  Files            56       56              
  Lines          1045      979      -66     
  Branches         70       63       -7     
============================================
- Hits            889      815      -74     
- Misses          125      133       +8     
  Partials         31       31

Flag	Coverage Δ
common	`73.68% <0.00%> (-1.77%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...stack/castor/common/entities/ArrayBackedTuple.java	`94.11% <ø> (ø)`
...r/client/download/DefaultCastorIntraVcpClient.java	`92.30% <ø> (-0.68%)`	⬇️
...castor/service/config/CastorServiceProperties.java	`91.66% <ø> (ø)`
...service/download/DefaultTuplesDownloadService.java	`93.54% <ø> (-1.69%)`	⬇️
...e/persistence/cache/CreateReservationSupplier.java	`100.00% <ø> (ø)`
...e/persistence/cache/ReservationCachingService.java	`94.89% <ø> (-0.87%)`	⬇️
...e/persistence/cache/UnlockReservationSupplier.java	`100.00% <ø> (ø)`
.../persistence/cache/WaitForReservationCallable.java	`72.22% <ø> (-1.47%)`	⬇️
...stence/fragmentstore/TupleChunkFragmentEntity.java	`92.50% <ø> (-0.36%)`	⬇️
...ragmentstore/TupleChunkFragmentStorageService.java	`91.52% <ø> (-0.42%)`	⬇️
... and 5 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eead173...31376bc. Read the comment docs.

sbckr · 2024-09-13T06:11:36Z

There should be separate branches for the individual features

sbckr

I stopped the review halfway through.

Please apologize, but it looks a bit messy. Please separate the features into individual branches and clean up the code.

I will continue afterwards.
Thank you

sbckr · 2024-09-16T08:55:41Z

castor-common/src/main/java/io/carbynestack/castor/common/entities/ArrayBackedTuple.java


 import io.carbynestack.castor.common.exceptions.CastorClientException;
+
+import java.io.ByteArrayOutputStream;


unused import?

If not update Copyright Header

sbckr · 2024-09-16T08:56:11Z

castor-common/src/main/java/io/carbynestack/castor/common/entities/TupleList.java

Please update Copyright header

sbckr · 2024-09-16T09:01:31Z

castor-common/src/main/java/io/carbynestack/castor/common/entities/TupleList.java

    }
  }

+  public byte[] toByteArray(){


This duplicates code from asChunk(UUID).
Please re-use this method accordingly.

sbckr · 2024-09-16T10:00:37Z

...client/src/main/java/io/carbynestack/castor/client/download/DefaultCastorIntraVcpClient.java

Please update Copyright header

sbckr · 2024-09-16T10:13:02Z

...client/src/main/java/io/carbynestack/castor/client/download/DefaultCastorIntraVcpClient.java

+                      byte[].class)
+              .get();
+      long length = tupleType.getTupleSize() * count;
+      return TupleList.fromStream(tupleType.getTupleCls(),


As Tuples as mostly consumed within a stream, it may make sense to keep tuples as a stream (potentially still wrapped as a TupleList and lazily converted later if required).

maybe TupleList became obsolete

sbckr · 2024-09-16T12:54:21Z

...rc/main/java/io/carbynestack/castor/service/persistence/cache/ReservationCachingService.java

+   * @throws InterruptedException : If the fragments can not be reserved
+   */
+  @Transactional()
+  public void storeReservationInDB(@NotNull Reservation reservation) throws InterruptedException {


Am I right that this method requires the reservation elements to be ordered by number of reserved Tuples where all elements with number of tuples < initial fragment size must be listed first.
That feels fragile especially as not properly documented (is it?)

sbckr · 2024-09-16T12:56:11Z

...rc/main/java/io/carbynestack/castor/service/persistence/cache/UnlockReservationSupplier.java

 import io.carbynestack.castor.common.exceptions.CastorServiceException;
 import java.util.function.Supplier;
+
+import io.micrometer.core.annotation.Timed;


unused import

sbckr · 2024-09-16T12:56:54Z

...c/main/java/io/carbynestack/castor/service/persistence/cache/WaitForReservationCallable.java

Update Copyright

sbckr · 2024-09-16T12:57:49Z

.../java/io/carbynestack/castor/service/persistence/fragmentstore/TupleChunkFragmentEntity.java

  static final String ACTIVATION_STATUS_COLUMN = "activation_status";
  static final String RESERVATION_ID_COLUMN = "reservation_id";
+  static final String VIEW_NAME = "distributed_fragments";
+  static final String POD_HASH_FIELD = "pod_hash";


some unused statics

sbckr · 2024-09-16T12:57:56Z

.../java/io/carbynestack/castor/service/persistence/fragmentstore/TupleChunkFragmentEntity.java

update copyright

Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

- transaction between two threads - now no longer checks for available tuples and will fail when trying to reserve instead, saving lots of time. Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

- improved reservation locking, still needds to be fixed Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

- now splits tuplechunks correctly if number of tuples % fragmentsize > 0. Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

sbckr · 2024-09-18T11:07:28Z

...vice/src/main/java/io/carbynestack/castor/service/download/DefaultTuplesDownloadService.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021 - for information on the respective copyright owner
+ * Copyright (c) 2024 - for information on the respective copyright owner


Copyright must be in format

...
Copyright (c) 2021 - 2024 - for information on the respective copyright owner
...

-> Copyright (c) [Year of file added] - [Year last modified] ...
if [Year of file added] is the same as [Year last modified] it is simply
-> Copyright (c) [Year of file added] ...

sbckr · 2024-09-18T13:08:16Z

...rc/main/java/io/carbynestack/castor/service/persistence/cache/CreateReservationSupplier.java

+              .retrieveSinglePartialFragment(
+                  tupleType, oddToReserve < castorServiceProperties.getInitialFragmentSize())
+              .orElseThrow(
+                  () -> new CastorServiceException(FAILED_FETCH_AVAILABLE_FRAGMENT_EXCEPTION_MSG));


Is there a test that ensures that all previous reservations are rolled back if there aren't enough tuples available

sbckr

see individual comments

sbckr · 2024-09-18T13:45:36Z

...rc/main/java/io/carbynestack/castor/service/persistence/cache/CreateReservationSupplier.java

+      TupleChunkFragmentEntity availableFragment =
+          fragmentStorageService
+              .retrieveSinglePartialFragment(
+                  tupleType, oddToReserve < castorServiceProperties.getInitialFragmentSize())


second argument preferSmall of retrieveSinglePartialFragment(TupleType tupleType, boolean preferSmall) is always true, does this make sense?

second argument preferSmall of retrieveSinglePartialFragment(TupleType tupleType, boolean preferSmall) is always true, does this make sense?

This argument is not always true. oddToReserve may be greater than the initial Fragment size, if Castor couldn't get the whole amount of Tuples in whole fragments. This happened in my test setup because Klyshko had an off-by-one error and would save 99,999 Tuples instead of 100,000 Tuples into a tupleChunk resulting in lots of tuples getting saved in partial fragments.

Line 79 adds the amount of tuples to oddToReserve that should have been reserved as round, couldn't.

sbckr · 2024-09-18T14:09:54Z

...rc/main/java/io/carbynestack/castor/service/persistence/cache/ReservationCachingService.java

+
+  /**
+   * Maps the reservation elements to tuplefragments and saves them accordingly in batches to the
+   * RDBMS. This function assumes that all 'non-round' fragments are saved before 'round' fragments


what means saved? in the database?

what means saved? in the database?

Yes. Saved means saved in this context (RDBMS) means saved to postgres.

sbckr · 2024-09-19T06:22:09Z

...est/java/io/carbynestack/castor/service/persistence/cache/ReservationCachingServiceTest.java

+    when(dedicatedTransactionServiceOptionalMock.isPresent()).thenReturn(true);
+    when(tupleChunkFragmentStorageServiceMock.retrieveSinglePartialFragment(tupleType, true))
+        .thenReturn(Optional.of(fragmentEntity));
+    // when(castorInterVcpClientSpy.shareReservation(any(Reservation.class))).thenReturn(false);


please keep code clean

CRAlwin added 4 commits July 31, 2024 11:05

perf!(service): Changed serialization of TupleLists

121ccc5

- intra-vcp/tuples endpoint now returns bytes instead of JSON (breaking change) - tupleList is serialized to bytes Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

perf: Directly return Bytes in intra-vcp/tuples

fb4103b

- Castor no longer uses the TupleList class and directly returns bytes retrieved from MinIO Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

refactor: Removed the timed annotation previously added

03e5493

Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

perf!: WIP of new reservation strategy

c47407e

sbckr commented Sep 16, 2024

View reviewed changes

CRAlwin added 8 commits September 18, 2024 11:34

fix: fixed wrong calculation of available fragments

1682218

Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

fix: reservation-locking:

06a413a

- transaction between two threads - now no longer checks for available tuples and will fail when trying to reserve instead, saving lots of time. Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

fix: reservation-locking:

67f48a6

- improved reservation locking, still needds to be fixed Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

fix: all concurrency issues are now resolved!

e103aad

Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

fix: websocketClient

2045236

- now splits tuplechunks correctly if number of tuples % fragmentsize > 0. Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

fix: now append round fragments at the end of the list

77ec75a

Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

fix: now reserves first tuple correctly; fixed some typos

d80e352

Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

fix: fixed all tests and added documentation

b5d99c3

Signed-off-by: Alwin Zomotor <alwin.zomotor@de.bosch.com>

CRAlwin force-pushed the optimize-tuple-management branch from 3ed574f to 516901d Compare September 18, 2024 09:35

fix: removed toByteArray function as it is now no longer needed

36badbd

CRAlwin force-pushed the optimize-tuple-management branch from 516901d to 36badbd Compare September 18, 2024 09:36

sbckr commented Sep 18, 2024

View reviewed changes

sbckr commented Sep 19, 2024

View reviewed changes


		import io.carbynestack.castor.common.exceptions.CastorClientException;

		import java.io.ByteArrayOutputStream;

Conversation

sbckr commented Aug 26, 2024

Uh oh!

codecov bot commented Aug 26, 2024

Codecov Report

Uh oh!

sbckr commented Sep 13, 2024

Uh oh!

sbckr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbckr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zalwin Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sbckr left a comment •

edited

Loading

zalwin Sep 25, 2024 •

edited

Loading